Skip to content

Commit

Permalink
Code review corrections
Browse files Browse the repository at this point in the history 
Signed-off-by: Gaël L'hopital <[email protected]>
  • Loading branch information
@clinique
clinique committed Sep 26, 2024
1 parent 09cd57e commit 5747ec8
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 38 deletions.
5 changes: 2 additions & 3 deletions ...ing.netatmo/src/main/java/org/openhab/binding/netatmo/internal/api/AuthenticationApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import static org.openhab.binding.netatmo.internal.api.data.NetatmoConstants.*;
import static org.openhab.core.auth.oauth2client.internal.Keyword.*;

import java.net.URI;
import java.util.List;
import java.util.Optional;
import java.util.Set;
Expand All @@ -37,8 +36,8 @@
*/
@NonNullByDefault
public class AuthenticationApi extends RestManager {
public static final URI TOKEN_URI = getApiBaseBuilder(PATH_OAUTH, SUB_PATH_TOKEN).build();
public static final URI AUTH_URI = getApiBaseBuilder(PATH_OAUTH, SUB_PATH_AUTHORIZE).build();
public static final String TOKEN_URI = getApiBaseBuilder(PATH_OAUTH, SUB_PATH_TOKEN).build().toString();
public static final String AUTH_URI = getApiBaseBuilder(PATH_OAUTH, SUB_PATH_AUTHORIZE).build().toString();

private List<Scope> grantedScope = List.of();
private @Nullable String authorization;
Expand Down
9 changes: 9 additions & 0 deletions ...mo/src/main/java/org/openhab/binding/netatmo/internal/config/ApiHandlerConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,13 @@ public class ApiHandlerConfiguration {
public String webHookUrl = "";
public String webHookPostfix = "";
public int reconnectInterval = 300;

public ConfigurationLevel check() {
if (clientId.isBlank()) {
return ConfigurationLevel.EMPTY_CLIENT_ID;
} else if (clientSecret.isBlank()) {
return ConfigurationLevel.EMPTY_CLIENT_SECRET;
}
return ConfigurationLevel.COMPLETED;
}
}
69 changes: 34 additions & 35 deletions ....netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/ApiBridgeHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Constructor;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.time.LocalDateTime;
Expand All @@ -35,6 +34,7 @@
import java.util.concurrent.TimeoutException;
import java.util.function.BiFunction;

import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.UriBuilder;

import org.eclipse.jdt.annotation.NonNullByDefault;
Expand Down Expand Up @@ -128,8 +128,7 @@ public ApiBridgeHandler(Bridge bridge, HttpClient httpClient, NADeserializer des
this.deserializer = deserializer;
this.httpService = httpService;
this.oAuthFactory = oAuthFactory;

requestCountChannelUID = new ChannelUID(thing.getUID(), GROUP_MONITORING, CHANNEL_REQUEST_COUNT);
this.requestCountChannelUID = new ChannelUID(thing.getUID(), GROUP_MONITORING, CHANNEL_REQUEST_COUNT);
}

@Override
Expand All @@ -138,22 +137,16 @@ public void initialize() {

ApiHandlerConfiguration configuration = getConfiguration();

if (configuration.clientId.isBlank()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
ConfigurationLevel.EMPTY_CLIENT_ID.message);
return;
}

if (configuration.clientSecret.isBlank()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
ConfigurationLevel.EMPTY_CLIENT_SECRET.message);
ConfigurationLevel confLevel = configuration.check();
if (!ConfigurationLevel.COMPLETED.equals(confLevel)) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, confLevel.message);
return;
}

oAuthClientService = oAuthFactory
.createOAuthClientService(this.getThing().getUID().getAsString(),
AuthenticationApi.TOKEN_URI.toString(), AuthenticationApi.AUTH_URI.toString(),
configuration.clientId, configuration.clientSecret, FeatureArea.ALL_SCOPES, false)
.createOAuthClientService(this.getThing().getUID().getAsString(), AuthenticationApi.TOKEN_URI,
AuthenticationApi.AUTH_URI, configuration.clientId, configuration.clientSecret,
FeatureArea.ALL_SCOPES, false)
.withGsonBuilder(new GsonBuilder().registerTypeAdapter(AccessTokenResponse.class,
new AccessTokenResponseDeserializer()));

Expand All @@ -170,15 +163,13 @@ public void openConnection(@Nullable String code, @Nullable String redirectUri)
logger.debug("Connecting to Netatmo API.");

ApiHandlerConfiguration configuration = getConfiguration();
if (!configuration.webHookUrl.isBlank()) {
SecurityApi securityApi = getRestManager(SecurityApi.class);
if (securityApi != null) {
webHookServlet.ifPresent(servlet -> servlet.dispose());
WebhookServlet servlet = new WebhookServlet(this, httpService, deserializer, securityApi,
configuration.webHookUrl, configuration.webHookPostfix);
servlet.startListening();
this.webHookServlet = Optional.of(servlet);
}
if (!configuration.webHookUrl.isBlank()
&& getRestManager(SecurityApi.class) instanceof SecurityApi securityApi) {
webHookServlet.ifPresent(servlet -> servlet.dispose());
WebhookServlet servlet = new WebhookServlet(this, httpService, deserializer, securityApi,
configuration.webHookUrl, configuration.webHookPostfix);
servlet.startListening();
this.webHookServlet = Optional.of(servlet);
}

updateStatus(ThingStatus.ONLINE);
Expand All @@ -201,8 +192,7 @@ private boolean authenticate(@Nullable String code, @Nullable String redirectUri
accessTokenResponse = oAuthClientService.getAccessTokenResponseByAuthorizationCode(code, redirectUri);

// Dispose grant servlet upon completion of authorization flow.
grantServlet.ifPresent(servlet -> servlet.dispose());
grantServlet = Optional.empty();
freeGrantServlet();
} else {
accessTokenResponse = oAuthClientService.getAccessTokenResponse();
}
Expand Down Expand Up @@ -241,7 +231,9 @@ public ApiHandlerConfiguration getConfiguration() {

private void prepareReconnection(int delay, @Nullable String message, @Nullable String code,
@Nullable String redirectUri) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, message);
if (!ThingStatus.OFFLINE.equals(thing.getStatus())) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, message);
}
connectApi.dispose();
freeConnectJob();
connectJob = Optional.of(scheduler.schedule(() -> openConnection(code, redirectUri), delay, TimeUnit.SECONDS));
Expand All @@ -253,15 +245,19 @@ private void freeConnectJob() {
connectJob = Optional.empty();
}

private void freeGrantServlet() {
grantServlet.ifPresent(servlet -> servlet.dispose());
grantServlet = Optional.empty();
}

@Override
public void dispose() {
logger.debug("Shutting down Netatmo API bridge handler.");

webHookServlet.ifPresent(servlet -> servlet.dispose());
webHookServlet = Optional.empty();

grantServlet.ifPresent(servlet -> servlet.dispose());
grantServlet = Optional.empty();
freeGrantServlet();

connectApi.dispose();
freeConnectJob();
Expand All @@ -286,13 +282,12 @@ public void handleCommand(ChannelUID channelUID, Command command) {
public <T extends RestManager> @Nullable T getRestManager(Class<T> clazz) {
if (!managers.containsKey(clazz)) {
try {
Constructor<T> constructor = clazz.getConstructor(getClass());
T instance = constructor.newInstance(this);
T instance = clazz.getConstructor(getClass()).newInstance(this);
Set<Scope> expected = instance.getRequiredScopes();
if (connectApi.matchesScopes(expected)) {
managers.put(clazz, instance);
} else {
logger.info("Unable to instantiate {}, expected scope {} is not active", clazz, expected);
logger.warn("Unable to instantiate {}, expected scope {} is not active", clazz, expected);
}
} catch (SecurityException | ReflectiveOperationException e) {
logger.warn("Error invoking RestManager constructor for class {}: {}", clazz, e.getMessage());
Expand All @@ -309,7 +304,7 @@ public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz,
Request request = httpClient.newRequest(uri).method(method).timeout(TIMEOUT_S, TimeUnit.SECONDS);

if (!authenticate(null, null)) {
prepareReconnection(getConfiguration().reconnectInterval, null, null, "@text/status-bridge-offline");
prepareReconnection(getConfiguration().reconnectInterval, "@text/status-bridge-offline", null, null);
throw new NetatmoException("Not authenticated");
}
connectApi.getAuthorization().ifPresent(auth -> request.header(HttpHeader.AUTHORIZATION, auth));
Expand All @@ -319,7 +314,7 @@ public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz,
InputStream stream = new ByteArrayInputStream(payload.getBytes(StandardCharsets.UTF_8));
try (InputStreamContentProvider inputStreamContentProvider = new InputStreamContentProvider(stream)) {
request.content(inputStreamContentProvider, contentType);
request.header(HttpHeader.ACCEPT, "application/json");
request.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON);
}
logger.trace(" -with payload: {} ", payload);
}
Expand Down Expand Up @@ -363,7 +358,11 @@ public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz,
"@text/maximum-usage-reached [ \"%d\" ]".formatted(API_LIMIT_INTERVAL_S), null, null);
}
throw exception;
} catch (InterruptedException | TimeoutException | ExecutionException e) {
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
throw new NetatmoException(e, "Request interrupted");
} catch (TimeoutException | ExecutionException e) {
if (retryCount > 0) {
logger.debug("Request error, retry counter: {}", retryCount);
return executeUri(uri, method, clazz, payload, contentType, retryCount - 1);
Expand Down

0 comments on commit 5747ec8

Please sign in to comment.