Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tr064] reduce network load and improve XML handling #9693

Merged
merged 4 commits into from
Jan 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.net.URI;
import java.net.URISyntaxException;
import java.time.Duration;
import java.util.*;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
Expand All @@ -42,6 +43,7 @@
import org.openhab.binding.tr064.internal.phonebook.PhonebookProvider;
import org.openhab.binding.tr064.internal.phonebook.Tr064PhonebookImpl;
import org.openhab.binding.tr064.internal.soap.SOAPConnector;
import org.openhab.binding.tr064.internal.soap.SOAPRequest;
import org.openhab.binding.tr064.internal.soap.SOAPValueConverter;
import org.openhab.binding.tr064.internal.util.SCPDUtil;
import org.openhab.binding.tr064.internal.util.Util;
Expand Down Expand Up @@ -81,8 +83,8 @@ public class Tr064RootHandler extends BaseBridgeHandler implements PhonebookProv

private final Map<ChannelUID, Tr064ChannelConfig> channels = new HashMap<>();
// caching is used to prevent excessive calls to the same action
private final ExpiringCacheMap<ChannelUID, State> stateCache = new ExpiringCacheMap<>(2000);
private Collection<Phonebook> phonebooks = Collections.emptyList();
private final ExpiringCacheMap<ChannelUID, State> stateCache = new ExpiringCacheMap<>(Duration.ofMillis(2000));
private Collection<Phonebook> phonebooks = List.of();

private @Nullable ScheduledFuture<?> connectFuture;
private @Nullable ScheduledFuture<?> pollFuture;
Expand Down Expand Up @@ -222,8 +224,8 @@ private boolean establishSecureConnectionAndUpdateProperties() {
this.deviceType = device.getDeviceType();

// try to get security (https) port
SOAPMessage soapResponse = soapConnector.doSOAPRequest(deviceService, "GetSecurityPort",
Collections.emptyMap());
SOAPMessage soapResponse = soapConnector
.doSOAPRequest(new SOAPRequest(deviceService, "GetSecurityPort"));
if (!soapResponse.getSOAPBody().hasFault()) {
SOAPValueConverter soapValueConverter = new SOAPValueConverter(httpClient);
soapValueConverter.getStateFromSOAPValue(soapResponse, "NewSecurityPort", null)
Expand All @@ -248,8 +250,8 @@ private boolean establishSecureConnectionAndUpdateProperties() {
"Could not get service definition for 'urn:DeviceInfo-com:serviceId:DeviceInfo1'"))
.getActionList().stream().filter(action -> action.getName().equals("GetInfo")).findFirst()
.orElseThrow(() -> new SCPDException("Action 'GetInfo' not found"));
SOAPMessage soapResponse1 = soapConnector.doSOAPRequest(deviceService, getInfoAction.getName(),
Collections.emptyMap());
SOAPMessage soapResponse1 = soapConnector
.doSOAPRequest(new SOAPRequest(deviceService, getInfoAction.getName()));
SOAPValueConverter soapValueConverter = new SOAPValueConverter(httpClient);
Map<String, String> properties = editProperties();
PROPERTY_ARGUMENTS.forEach(argumentName -> getInfoAction.getArgumentList().stream()
Expand All @@ -276,7 +278,7 @@ private boolean establishSecureConnectionAndUpdateProperties() {
*/
public List<SCPDDeviceType> getAllSubDevices() {
final SCPDUtil scpdUtil = this.scpdUtil;
return (scpdUtil == null) ? Collections.emptyList() : scpdUtil.getAllSubDevices();
return (scpdUtil == null) ? List.of() : scpdUtil.getAllSubDevices();
}

/**
Expand Down Expand Up @@ -334,8 +336,8 @@ private Collection<Phonebook> processPhonebookList(SOAPMessage soapMessagePhoneb
.map(phonebookList -> Arrays.stream(phonebookList.toString().split(","))).orElse(Stream.empty())
.map(index -> {
try {
SOAPMessage soapMessageURL = soapConnector.doSOAPRequest(scpdService, "GetPhonebook",
Map.of("NewPhonebookID", index));
SOAPMessage soapMessageURL = soapConnector.doSOAPRequest(
new SOAPRequest(scpdService, "GetPhonebook", Map.of("NewPhonebookID", index)));
return soapValueConverter.getStateFromSOAPValue(soapMessageURL, "NewPhonebookURL", null)
.map(url -> (Phonebook) new Tr064PhonebookImpl(httpClient, url.toString()));
} catch (Tr064CommunicationException e) {
Expand All @@ -357,12 +359,12 @@ private void retrievePhonebooks() {

phonebooks = scpdService.map(service -> {
try {
return processPhonebookList(
soapConnector.doSOAPRequest(service, "GetPhonebookList", Collections.emptyMap()), service);
return processPhonebookList(soapConnector.doSOAPRequest(new SOAPRequest(service, "GetPhonebookList")),
service);
} catch (Tr064CommunicationException e) {
return Collections.<Phonebook> emptyList();
}
}).orElse(Collections.emptyList());
}).orElse(List.of());

if (phonebooks.isEmpty()) {
logger.warn("Could not get phonebooks for thing {}", thing.getUID());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.time.Duration;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand Down Expand Up @@ -57,12 +59,15 @@
*/
@NonNullByDefault
public class SOAPConnector {
private static final int SOAP_TIMEOUT = 2000; // in ms
private static final int SOAP_TIMEOUT = 5; // in
private final Logger logger = LoggerFactory.getLogger(SOAPConnector.class);
private final HttpClient httpClient;
private final String endpointBaseURL;
private final SOAPValueConverter soapValueConverter;

private final ExpiringCacheMap<SOAPRequest, SOAPMessage> soapMessageCache = new ExpiringCacheMap<>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a minor remark. Why not passing the time value directly like here ? Or use Duration.of() to improve readability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stateCache can probably be omitted. Sinc ethe SOAP request to fill it is cached anyway. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we probably could remove that one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, it bypasses the construction of the SOAP request and does not add much complexity now that it is here. So I would prefer to leave it in place.

Duration.ofMillis(2000));

public SOAPConnector(HttpClient httpClient, String endpointBaseURL) {
this.httpClient = httpClient;
this.endpointBaseURL = endpointBaseURL;
Expand All @@ -72,15 +77,12 @@ public SOAPConnector(HttpClient httpClient, String endpointBaseURL) {
/**
* prepare a SOAP request for an action request to a service
*
* @param service the service
* @param soapAction the action to send
* @param arguments arguments to send along with the request
* @param soapRequest the request to be generated
* @return a jetty Request containing the full SOAP message
* @throws IOException if a problem while writing the SOAP message to the Request occurs
* @throws SOAPException if a problem with creating the SOAP message occurs
*/
private Request prepareSOAPRequest(SCPDServiceType service, String soapAction, Map<String, String> arguments)
throws IOException, SOAPException {
private Request prepareSOAPRequest(SOAPRequest soapRequest) throws IOException, SOAPException {
MessageFactory messageFactory = MessageFactory.newInstance();
SOAPMessage soapMessage = messageFactory.createMessage();
SOAPPart soapPart = soapMessage.getSOAPPart();
Expand All @@ -89,8 +91,9 @@ private Request prepareSOAPRequest(SCPDServiceType service, String soapAction, M

// SOAP body
SOAPBody soapBody = envelope.getBody();
SOAPElement soapBodyElem = soapBody.addChildElement(soapAction, "u", service.getServiceType());
arguments.entrySet().stream().sorted(Map.Entry.comparingByKey()).forEach(argument -> {
SOAPElement soapBodyElem = soapBody.addChildElement(soapRequest.soapAction, "u",
soapRequest.service.getServiceType());
soapRequest.arguments.entrySet().stream().sorted(Map.Entry.comparingByKey()).forEach(argument -> {
try {
soapBodyElem.addChildElement(argument.getKey()).setTextContent(argument.getValue());
} catch (SOAPException e) {
Expand All @@ -101,11 +104,12 @@ private Request prepareSOAPRequest(SCPDServiceType service, String soapAction, M

// SOAP headers
MimeHeaders headers = soapMessage.getMimeHeaders();
headers.addHeader("SOAPAction", service.getServiceType() + "#" + soapAction);
headers.addHeader("SOAPAction", soapRequest.service.getServiceType() + "#" + soapRequest.soapAction);
soapMessage.saveChanges();

// create Request and add headers and content
Request request = httpClient.newRequest(endpointBaseURL + service.getControlURL()).method(HttpMethod.POST);
Request request = httpClient.newRequest(endpointBaseURL + soapRequest.service.getControlURL())
.method(HttpMethod.POST);
((Iterator<MimeHeader>) soapMessage.getMimeHeaders().getAllHeaders())
.forEachRemaining(header -> request.header(header.getName(), header.getValue()));
try (final ByteArrayOutputStream os = new ByteArrayOutputStream()) {
Expand All @@ -118,19 +122,46 @@ private Request prepareSOAPRequest(SCPDServiceType service, String soapAction, M
}

/**
* execute a SOAP request
* execute a SOAP request with cache
*
* @param service the service to send the action to
* @param soapAction the action itself
* @param arguments arguments to send along with the request
* @param soapRequest the request itself
* @return the SOAPMessage answer from the remote host
* @throws Tr064CommunicationException if an error occurs during the request
*/
public synchronized SOAPMessage doSOAPRequest(SCPDServiceType service, String soapAction,
Map<String, String> arguments) throws Tr064CommunicationException {
public SOAPMessage doSOAPRequest(SOAPRequest soapRequest) throws Tr064CommunicationException {
try {
Request request = prepareSOAPRequest(service, soapAction, arguments).timeout(SOAP_TIMEOUT,
TimeUnit.MILLISECONDS);
SOAPMessage soapMessage = Objects.requireNonNull(soapMessageCache.putIfAbsentAndGet(soapRequest, () -> {
J-N-K marked this conversation as resolved.
Show resolved Hide resolved
try {
SOAPMessage newValue = doSOAPRequestUncached(soapRequest);
logger.trace("Storing in cache: {}", newValue);
return newValue;
} catch (Tr064CommunicationException e) {
// wrap exception
throw new IllegalArgumentException(e);
}
}));
logger.trace("Returning from cache: {}", soapMessage);
return soapMessage;
} catch (IllegalArgumentException e) {
Throwable cause = e.getCause();
if (cause instanceof Tr064CommunicationException) {
throw (Tr064CommunicationException) cause;
} else {
throw e;
}
}
}

/**
* execute a SOAP request without cache
*
* @param soapRequest the request itself
* @return the SOAPMessage answer from the remote host
* @throws Tr064CommunicationException if an error occurs during the request
*/
public synchronized SOAPMessage doSOAPRequestUncached(SOAPRequest soapRequest) throws Tr064CommunicationException {
try {
Request request = prepareSOAPRequest(soapRequest).timeout(SOAP_TIMEOUT, TimeUnit.SECONDS);
if (logger.isTraceEnabled()) {
request.getContent().forEach(buffer -> logger.trace("Request: {}", new String(buffer.array())));
}
Expand All @@ -140,8 +171,7 @@ public synchronized SOAPMessage doSOAPRequest(SCPDServiceType service, String so
// retry once if authentication expired
logger.trace("Re-Auth needed.");
httpClient.getAuthenticationStore().clearAuthenticationResults();
request = prepareSOAPRequest(service, soapAction, arguments).timeout(SOAP_TIMEOUT,
TimeUnit.MILLISECONDS);
request = prepareSOAPRequest(soapRequest).timeout(SOAP_TIMEOUT, TimeUnit.SECONDS);
response = request.send();
}
try (final ByteArrayInputStream is = new ByteArrayInputStream(response.getContent())) {
Expand Down Expand Up @@ -186,7 +216,9 @@ public void sendChannelCommandToDevice(Tr064ChannelConfig channelConfig, Command
channelConfig.getChannelTypeDescription().getGetAction().getParameter().getName(),
parameter);
}
doSOAPRequest(service, channelTypeDescription.getSetAction().getName(), arguments);
SOAPRequest soapRequest = new SOAPRequest(service,
channelTypeDescription.getSetAction().getName(), arguments);
J-N-K marked this conversation as resolved.
Show resolved Hide resolved
doSOAPRequestUncached(soapRequest);
} catch (Tr064CommunicationException e) {
logger.warn("Could not send command {}: {}", command, e.getMessage());
}
Expand Down Expand Up @@ -224,8 +256,8 @@ public State getChannelStateFromDevice(final Tr064ChannelConfig channelConfig,
if (parameter != null && !action.getParameter().isInternalOnly()) {
arguments.put(action.getParameter().getName(), parameter);
}
SOAPMessage soapResponse = doSOAPRequest(channelConfig.getService(), getAction.getName(), arguments);

SOAPMessage soapResponse = doSOAPRequest(
new SOAPRequest(channelConfig.getService(), getAction.getName(), arguments));
String argumentName = channelConfig.getChannelTypeDescription().getGetAction().getArgument();
// find all other channels with the same action that are already in cache, so we can update them
Map<ChannelUID, Tr064ChannelConfig> channelsInRequest = channelConfigMap.entrySet().stream()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* Copyright (c) 2010-2021 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.binding.tr064.internal.soap;

import java.util.Map;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.tr064.internal.dto.scpd.root.SCPDServiceType;

/**
* The {@link SOAPRequest} is a wrapper for SOAP requests
*
* @author Jan N. Klug - Initial contribution
*/
@NonNullByDefault
public class SOAPRequest {
public SCPDServiceType service;
public String soapAction;
public Map<String, String> arguments = Map.of();

public SOAPRequest(SCPDServiceType service, String soapAction) {
this.service = service;
this.soapAction = soapAction;
}

public SOAPRequest(SCPDServiceType service, String soapAction, Map<String, String> arguments) {
this.service = service;
this.soapAction = soapAction;
this.arguments = arguments;
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;

SOAPRequest that = (SOAPRequest) o;

if (!service.equals(that.service))
return false;
if (!soapAction.equals(that.soapAction))
return false;
return arguments.equals(that.arguments);
}

@Override
public int hashCode() {
int result = service.hashCode();
result = 31 * result + soapAction.hashCode();
result = 31 * result + arguments.hashCode();
return result;
}

@Override
public String toString() {
return "SOAPRequest{" + "service=" + service + ", soapAction='" + soapAction + '\'' + ", arguments=" + arguments
+ '}';
}
}
Loading